Skip to content

SQL Server to Postgres Migration#7547

Open
labkey-martyp wants to merge 5 commits intorelease26.3-SNAPSHOTfrom
26.3_fb_tnprc_migration
Open

SQL Server to Postgres Migration#7547
labkey-martyp wants to merge 5 commits intorelease26.3-SNAPSHOTfrom
26.3_fb_tnprc_migration

Conversation

@labkey-martyp
Copy link
Copy Markdown
Contributor

@labkey-martyp labkey-martyp commented Apr 3, 2026

Rationale

  1. age_in_days / age(..., SQL_TSI_DAY) — Preserves SQL Server's calendar-boundary semantics for timestampdiff(SQL_TSI_DAY, ...) on Postgres, where the native operator counts 24-hour intervals instead. Consistent with the existing age_in_months/age_in_years functions; permanent addition to LabKey SQL.
  2. CAST(... AS BOOLEAN) in SELECT lists (QuerySelect.java) — On SQL Server, boolean QMethodCalls in a select list are wrapped in CASE WHEN (...) THEN 1 ELSE 0 END to avoid predicates as scalar values. But CAST(x AS BIT) is already a value, not a predicate, and SQL Server rejects the wrapped form. Fix excludes ConvertInfo from the wrap. (repro: insert failing query / error)
  3. DatabaseMigrationService.registerSchemaContributor — Interface for DatabaseMigrationService.registerSchemaContributor in premiumModules

Related Pull Requests

Changes

  • Method.java — age_in_days method, age(..., SQL_TSI_DAY) dispatch, updated validation
  • QueryTestCase.jsp — tests for both new syntaxes including datetime calendar-boundary cases
  • LabKeySql.md — documents age_in_days
  • QuerySelect.java — skip boolean wrap for CAST/CONVERT on SQL Server
  • DatabaseMigrationService.java — new registerSchemaContributor interface method

Copy link
Copy Markdown
Contributor

@labkey-adam labkey-adam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review suggestions from me and Claude. I also suggest running Claude /review_pr after changes. If its findings are off, consider adding comments that address.

new MethodSqlTest("SELECT TIMESTAMPDIFF2(SQL_TSI_YEAR, CAST('15 Jan 2003' AS TIMESTAMP), CAST('15 Jan 2006' AS TIMESTAMP))", JdbcType.INTEGER, 3),
new MethodSqlTest("SELECT TIMESTAMPDIFF2(SQL_TSI_YEAR, CAST('14 Jan 2006' AS TIMESTAMP), CAST('15 Jan 2003' AS TIMESTAMP))", JdbcType.INTEGER, -2),
new MethodSqlTest("SELECT TIMESTAMPDIFF2(SQL_TSI_FRAC_SECOND, CAST('01 Jan 2004 5:00:00' AS TIMESTAMP), CAST('01 Jan 2004 5:00:01' AS TIMESTAMP))", JdbcType.BIGINT, 1000),
new MethodSqlTest("SELECT TIMESTAMPDIFF2(SQL_TSI_FRAC_SECOND, CAST('01 Jan 2004' AS TIMESTAMP), CAST('31 Jan 2004' AS TIMESTAMP))", JdbcType.BIGINT, 2592000000L),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test for age_in_days()?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And new age() option?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@labkey-martyp
Copy link
Copy Markdown
Contributor Author

@labkey-adam After a little further testing, it turns out we don't really need timestampdiff2 for this migration. age_in_days doesn't have the parameter swapping issue and will work for all use cases. I also added tests for age_in_days. /review-pr is just returning a few low severity findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants